Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to winit 0.29 #3649

Merged
merged 51 commits into from
Dec 18, 2023
Merged

Update to winit 0.29 #3649

merged 51 commits into from
Dec 18, 2023

Conversation

fornwall
Copy link
Contributor

@fornwall fornwall commented Nov 27, 2023

@fornwall fornwall mentioned this pull request Nov 27, 2023
@fornwall
Copy link
Contributor Author

fornwall commented Nov 27, 2023

Current status: At least on mac, running cargo run --release -p egui_demo_app brings up the window and it can be resized, but not interacted with.

crates/egui-winit/src/lib.rs Outdated Show resolved Hide resolved
crates/egui-winit/src/lib.rs Outdated Show resolved Hide resolved
@emilk emilk added eframe Relates to epi and eframe egui-winit porblems related to winit labels Nov 28, 2023
@PingPongun
Copy link
Contributor

I prepared patch that updates resvg to v0.36 (issue: #3652):
update_resvg_0_36.patch

I think it matches in this PR as:

  • both resvg and winit (through sctk-adwaita) depend on tiny-skia, so updating them separately results in dependency duplication and cargo-deny fail.
  • there were already some small changes related to svg (commits b4e6246 & 7c89b67) and deny.toml (0dbd597) in this PR

In example images ferris is scaled differently, but I guess that now it scales in expected way (takes all available space; before this patch it takes up to space that, was available at first render- it does not upscale).

Digression 1

This patch is minimal adaptation to new resvg api and small related simplification, however it should be considered to update loaders (currently if svg image initially was small and was scaled up it will be blurred, see #3501). As svg image can now scale over render size, problem will be more often seen now.

(currently SvgLoader theoretically should rerender for different sizes (but will result in memory leak in that case), but refreshing is stopped earlier in DefaultTextureLoader).

I have some ideas on updating loaders (it will fix issue with svg and enable reloading image if file has been changed):

  • add two more variants to ImagePoll/BytesPoll (Changing(old_data) & Changed(new_data))

  • change order of loader tasks (currently is: 1. check if loader has image cached, 2. if not call lower level loader -> change to -> 1. call lower level loader 2. if lower level is not changed load cached)

  • SvgLoader:

    • store resvg::Tree & SizeHint in svg_loader::Entry

    • remove SizeHint from SvgLoader hashmap key

    • one of the following:

      • rerender svg if image size has been changed,
      • render dynamically (first approach will equal second one if there are multiple images from the same file but in different scale),
      • rerender svg if new image size is larger than rendered one (I guess this will be most performant, but there may be some minor down-scalling artifacts)

As these will require changes that are completely unrelated to winit, I guess it will better fit into separate PR, after this PR is merged.

Digression 2: check.sh installs cranky, but does not install typos nor cargo-deny that are also needed

@emilk
Copy link
Owner

emilk commented Nov 30, 2023

@PingPongun thanks for the patch, but let's wait with the resvg update for a separate PR - this one is big enough as is 😓

@YgorSouza
Copy link
Contributor

I was testing to see if this branch fixes #3673, which it does, but on my machine it makes the main viewport on the demo app stop repainting whenever it is in the foreground and the extra viewport is open. The person who opened the issue did not observe the same behavior on their machine, which has a similar but different setup.

winit_029_demo_app.mp4

Here is my setup:

OS: Fedora Linux 38 (Workstation Edition) x86_64 
Host: Laptop (12th Gen Intel Core) A6 
Kernel: 6.6.2-101.fc38.x86_64 
Resolution: 2256x1504 
DE: GNOME 44.6 
WM: Mutter 
WM Theme: Adwaita 
Theme: Adwaita-dark [GTK2/3] 
Icons: Adwaita [GTK2/3] 
CPU: 12th Gen Intel i7-1260P (16) @ 4.700GHz 
GPU: Intel Alder Lake-P GT2 [Iris Xe Graphics] 

@YgorSouza
Copy link
Contributor

Another problem I found: the modifier keys are ignored. Both the left and the right ones. They don't appear as down in the inspection window in the demo app, and they don't have any other effect either (aside from making text upper case, in the case of the shift key).

@ghost ghost mentioned this pull request Dec 5, 2023
@emilk
Copy link
Owner

emilk commented Dec 12, 2023

According to @Wumpf this seems to work pretty well on Windows now.

@emilk
Copy link
Owner

emilk commented Dec 18, 2023

IME seems to work on my Mac.

Are there any more blockers to merging this?

@Vickerinox
Copy link

Vickerinox commented Dec 18, 2023

Just tried it out on Linux Mint 21.2, everything seems to be working perfectly aside from window dragging, which causes the window to fly into the top left corner of the screen (see video) I've tested to make sure every letter on my keyboard works + modifiers, as well as the various demos in the demo app. Nothing obvious that's different aside form the previously mentioned bug.

out.mp4

@emilk
Copy link
Owner

emilk commented Dec 18, 2023

Thanks @Vickerinox - I think that is an acceptable level of regression. If anyone wants that feature to work they can make a PR :)

@emilk emilk merged commit 8e5959d into emilk:master Dec 18, 2023
19 checks passed
emilk pushed a commit that referenced this pull request Jan 17, 2024
<!--
Please read the "Making a PR" section of
[`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md)
before opening a Pull Request!

* Keep your PR:s small and focused.
* If applicable, add a screenshot or gif.
* If it is a non-trivial addition, consider adding a demo for it to
`egui_demo_lib`, or a new example.
* Do NOT open PR:s from your `master` branch, as that makes it hard for
maintainers to add commits to your PR.
* Remember to run `cargo fmt` and `cargo cranky`.
* Open the PR as a draft until you have self-reviewed it and run
`./scripts/check.sh`.
* When you have addressed a PR comment, mark it as resolved.

Please be patient! I will review your PR, but my time is limited!
-->

This breaking change seems to have snuck in during #3649. Previously it
would never consume these, today it consumes them unconditionally, and
with this change it'll ~~only consume them if egui wants text input~~
never consume them.

This is a blocker for updating our application, sadly :(
TicClick added a commit to TicClick/egui that referenced this pull request May 5, 2024
this partly restores event-emitting behaviour to the state before emilk#3649, when shortcuts such as Ctrl-C used to work regardless of the active layout. the difference is that physical keys are only used in case of the logical ones' absence now among the named keys.
emilk pushed a commit that referenced this pull request May 10, 2024
…ve (#4461)

resolves #4081 (see discussion
starting from
#3653 (comment) for
extra context)

this partly restores event-emitting behaviour to the state before #3649,
when shortcuts such as `Ctrl` + `C` used to work regardless of the
active layout. the difference is that physical keys are only used in
case of the logical ones' absence now among the named keys.

while originally I have only limited this to clipboard shortcuts
(Ctrl+C/V/X), honestly it felt like a half-assed solution. as a result,
I decided to expand this behaviour to all key events to stick to the
original logic, in case there are other workflows and hotkeys people
rely on or expect to work out of the box. let me know if this is an
issue.
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
…ve (emilk#4461)

resolves emilk#4081 (see discussion
starting from
emilk#3653 (comment) for
extra context)

this partly restores event-emitting behaviour to the state before emilk#3649,
when shortcuts such as `Ctrl` + `C` used to work regardless of the
active layout. the difference is that physical keys are only used in
case of the logical ones' absence now among the named keys.

while originally I have only limited this to clipboard shortcuts
(Ctrl+C/V/X), honestly it felt like a half-assed solution. as a result,
I decided to expand this behaviour to all key events to stick to the
original logic, in case there are other workflows and hotkeys people
rely on or expect to work out of the box. let me know if this is an
issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file eframe Relates to epi and eframe egui-winit porblems related to winit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to winit 0.29 Add non-portable keys to egui::Key Forward scancodes via egui-winit